Skip to content

pwd: fix hostname resolution on macos #7029

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

reykjalin
Copy link
Contributor

@reykjalin reykjalin commented Apr 8, 2025

Description

Yet another edge case in #2484

When macOS's "Private WiFi address" feature is enabled it'll change the hostname to a mac address. Mac addresses look like URIs with a hostname and port component, e.g. 12:34:56:78:90:12 where :12 looks like port 12. However, mac addresses use hex numbers and as such can also contain letters a through f. So, a mac address like ab:cd:ef:ab:cd:ef is valid, but will not be parsed as a URI, because :ef is not a valid port.

This commit attempts to fix that by checking if the hostname is a valid mac address when std.Uri.parse() fails and constructing a new std.Uri struct using that information.

It's not perfect, but is equally compliant with the URI spec as std.Uri currently is. Meaning not at all compliant 😅

Testing instructions

Unit tests

Important

I don't know if these tests are run in CI or if they're picked up by zig build test. I get an unrelated crash that mentions minidump and an invalid OSC command when I try to run zig build test on my mac.

  1. Make sure zig test src/os/hostname.zig is passing.

Manual testing instructions

Setup - Enable the "Private WiFi address" setting

Important

You must be connected to WiFi to be able to test this.

  1. Open your mac's "System Settings".
  2. Go to Network → Wi-Fi → Details.
image
  1. Set the "Private Wi-Fi address" setting to Rotating.
image

Important

Now you wait. The private Wi-Fi address will eventually rotate to a mac address that ends with a non-digit, e.g. 0a, ff, e2, etc. You'll notice this when your shell integration stops working, e.g. you open a new tab in Ghostty and the shell is in your home directory instead of whichever directory you had open in your previous tab.

Testing the changes

  1. Open Ghostty.
  2. cd to any directory that isn't the default (usually $HOME) directory, e.g. cd Documents.
  3. Open a new tab (Cmd+T) or split (Cmd+D).
  4. Assuming the setup steps have been followed you should:
    • On main: land in $HOME in the new tab or split.
    • On this branch: land in the same working directory as the original tab or split.

Copy link
Collaborator

@jparise jparise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two quick thoughts:

  1. This is probably getting complicated enough to warrant a separate utility function (with tests), perhaps outside of the stream handler.
  2. I'm starting to feel like we should follow Kitty's example and treat the file:// and kitty-report-cwd:// schemes differently: only file:// URLs get the encoding. (See #7033)

@reykjalin
Copy link
Contributor Author

This is probably getting complicated enough to warrant a separate utility function (with tests), perhaps outside of the stream handler.

100% agree, I had the same thought as I was working on this. I haven't confirmed the fix though so I figured I'd make a quick and dirty version that I could test, and clean it up later. And yep, that includes a separate function, tests, the whole thing 🙂

I'm starting to feel like we should follow Kitty's example and treat the file:// and kitty-report-cwd:// schemes differently: only file:// URLs get the encoding.

Yeah, we probably should. Or at the very least we probably shouldn't be using std.Uri here. It's such an incomplete implementation at this point that we'd probably be better served making our own parser here 🤔

For example, percent_encoding isn't actually enforced in std.Uri, it just assumes it. There's no escaping or cleaning done to the path to make sure it's safe, etc. That shouldn't necessarily be part of std.Uri, but I think those are things we should be doing here, so we'll want our own parser here eventually anyway 🤔

@jcollie
Copy link
Member

jcollie commented Apr 8, 2025

It seems to me that this would be better solved on the macOS level. Either by using scutil to force a "real" hostname or modifying the shell integration to inject a better hostname into the OSC 7 escape.

@reykjalin
Copy link
Contributor Author

It seems to me that this would be better solved on the macOS level. Either by using scutil to force a "real" hostname or modifying the shell integration to inject a better hostname into the OSC 7 escape.

I think the main problem is that posix.gethostname() - the hostname we compare to whatever the shell integration passes to us - returns the mac address version of the hostname.

I suppose we could run scutil --get LocalHostName on macOS instead of calling posix.gethostname() and make sure the shell integration is getting a similar value for us to compare 🤔
But it also feels really bad to have to run an external executable on macOS to get this working right.

And either way I think we should treat mac addresses as valid hostnames, even if they're not compliant with the general linux hostname spec of [a-z0-9] and a hyphen (-) due to using colons (:). This feature is enabled by default in macOS, and it isn't that big of a lift to support specifically only valid mac addresses, even if it does complicate this particular part of the codebase somewhat.

@reykjalin
Copy link
Contributor Author

reykjalin commented May 1, 2025

Finally got to a place where I can reproduce it and recorded a video just to demonstrate:

ghostty-bug.mp4

@reykjalin reykjalin force-pushed the fix-invalid-uri-in-hostname-on-macos branch from d372a1f to a3c7b3f Compare May 1, 2025 03:58
@reykjalin
Copy link
Contributor Author

And now to demonstrate that the current set of changes work:

ghostty-fix.mp4

@reykjalin
Copy link
Contributor Author

I still need to clean up the code here more before this is ready. I'm thinking of creating some sort of utility function in src/os/hostname.zig that will extract a hostname from a file url string passed to it. We can then add some tests for that function so we can be sure we're doing this right.

It's way too late here to do that now though, so I'll pick this up again when I have the time, hopefully in the next week or so.

@reykjalin reykjalin force-pushed the fix-invalid-uri-in-hostname-on-macos branch from b780820 to f6d1718 Compare May 9, 2025 02:53
@reykjalin reykjalin requested a review from jparise May 9, 2025 03:02
@reykjalin reykjalin marked this pull request as ready for review May 9, 2025 03:02
@reykjalin
Copy link
Contributor Author

@jparise I think this is ready for a proper review now :)

@00-kat 00-kat added the os/macos label May 9, 2025
@reykjalin reykjalin requested a review from jparise May 14, 2025 03:44
Copy link
Collaborator

@jparise jparise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me but deferring to others who have more regular maintainership over these files.

reykjalin added 13 commits June 7, 2025 22:07
When macOS's "Private WiFi address" feature is enabled it'll change the
hostname to a mac address. Mac addresses look like URIs with a hostname
and port component, e.g. 12:34:56:78:90:12 where `:12` looks like port
12. However, mac addresses can also contain letters a through f, so a
valid mac address like ab:cd:ef:ab:cd:ef is valid, but will not be parsed
as a URI, because `:ef` is not a valid port.

This commit attempts to fix that by checking if the hostname is a valid
mac address when `std.Uri.parse()` fails and constructing a new std.Uri
struct using that information.

It's not perfect, but is equally compliant with the URI spec as std.Uri
currently is.
Also adds a test to verify that the function is working as intended.
we only need the mac-address-as-hostname workaround on macos, so we
now have a comptime check to see if we're on macos.
@reykjalin reykjalin force-pushed the fix-invalid-uri-in-hostname-on-macos branch from d6b99a6 to e4a175d Compare June 8, 2025 02:07
@reykjalin reykjalin requested a review from jparise June 8, 2025 02:19
@reykjalin
Copy link
Contributor Author

Sorry for the long response time here. I've rebased the branch on main and addressed the previous review, so this should be ready for another review 🎉

@reykjalin reykjalin requested a review from jparise June 13, 2025 15:20
return std.Uri.parse(url) catch |e| {
// The mac-address-as-hostname issue is specific to macOS so we just return an error if we
// hit it on other platforms.
if (comptime builtin.os.tag != .macos) return e;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jparise I changed this from comptime if (… to if (comptime … to fix the build failure on non macOS systems, but I'm not familiar enough with Zig's comptime to know if it makes sense to even keep the comptime check in here? Should we just remove it, or does it still make sense to keep it? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In its current state, I don't think we need to gate this on macOS. It's done in a generic way, although it would be good to get some real-world confirmation that this all makes sense on other platforms (e.g. no false positives/negatives).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a very simple test run in a Fedora VM and this seemed to be working correctly. It was the same thing I did in #7029 (comment) to confirm this was was working correctly. Super simple test so I'm not sure if we want something more thorough.

Anything I can do here to help with testing this on other platforms?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcollie and @pluiedev, do you have any opinions on how this will play on Linux?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely no clue

@reykjalin reykjalin requested a review from jparise June 18, 2025 16:45
@mitchellh mitchellh merged commit 0b5092b into ghostty-org:main Jun 24, 2025
38 checks passed
@github-actions github-actions bot added this to the 1.2.0 milestone Jun 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants